Skip to content

Conversation

@prwhelan
Copy link
Member

@prwhelan prwhelan commented Nov 6, 2025

Search when MinimizeRoundTrip (MRT) and Cross-Project Search (CPS) are true. This will fan out to call ResolveIndexAction on each resolved LinkedProject. Results are merged and validated based on ignore_unavailable and allow_no_indices.

The results are used as the new resolvedIndices when issuing the subsequent search requests. Linked projects will be searched using the original index express.

@prwhelan prwhelan added >non-issue Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/Search Catch all for Search Foundations labels Nov 6, 2025
Copy link
Contributor

@quux00 quux00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial feedback after first round review.

@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Nov 20, 2025
Copy link
Contributor

@quux00 quux00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress. Overall very clean code. This was a first pass review. I'll want to dive deep on the next round also once the first round of feedback is discussed.

@prwhelan prwhelan marked this pull request as ready for review November 24, 2025 17:20
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

Copy link
Contributor

@quux00 quux00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on removing the local resolution via _resolve/index. That part looks solid.

I left a few questions/suggestions that are still confusing me.


originalResolvedIndices.getRemoteClusterIndices()
.forEach(
(projectName, projectIndices) -> resolveRemoteCrossProjectIndex(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a follow-on PR, I think we'll need to build some tests that simulate the case where one of the remote clusters is not available (not responding) and hits the timeout. We would do those tests in the serverless IT test and we can use some of the test plugin features we have in the core CCS IT tests to induce non-responsive linked projects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a plugin for it. It is achievable by adding a "handling behaviour" to the underlying MockTransportService.

/**
* Only used for ccs_minimize_roundtrips=true pathway
*/
private void mergeResolvedIndices(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional) - I think you could make this method static (and non-private) so that you could write unit tests for it.

That may not be worth it since it does it 3 things and two of those (CrossProjectIndexResolutionValidator.validate and ResolvedIndices.resolveWithIndexExpressions) are already static methods with tests.

new OriginalIndices(new String[] { "some-local-index" }, IndicesOptions.DEFAULT),
Map.of(mock(), mock()),
remoteExpressions,
IndicesOptions.DEFAULT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the IndicesOptions we'll be passing in for CPS? If not, should we change it to match what the production code passes in (which I think would be from indicesOptionsForCrossProjectFanout)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the production code, it's resolutionIdxOpts, so that's the CPS options. So, yes, it'd be wiser to use them here.

Copy link
Member Author

@prwhelan prwhelan Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it passes as-is - it can be any value

if it needs to be a specific value, should we add validation or an assertion (to both MRT = false and MRT = true paths)?


/**
* Create a new {@link ResolvedIndices} instance from a Map of Projects to {@link ResolvedIndexExpressions}.
* This method guarantees that the resulting remote project contains at least one index resolved for the mapped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I'm missing something. This statement:

This method guarantees that the resulting remote project contains at least one index resolved for the mapped ResolvedIndexExpressions

doesn't seem right to me. The check that guarantees this is the call to CrossProjectIndexResolutionValidator.validate in mergeResolvedIndices, isn't it?

Copy link
Contributor

@pawankartik-elastic pawankartik-elastic Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a guarantee on the map's (remoteIndices) structuring rather than a behaviour ascertaining. Either way, my opinion is that it's not immediately clear what the doc means, unless you're by default familiar with the CPS specifics. Could this be rewritten or clarified?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 268 through 280 iterates over a map of ResolvedIndexExpressions and filters the internal list of ResolvedIndexExpression for entities that have .localIndexResolutionResult() == SUCCESS and at least one index.

For example, if project-1 had the original index expression logs* map to indices ["logs-1", "logs-2"], then the result has project-1 map to ["logs*"]. If project-1 had logs* map to [], then project-1 is not included in the resulting map.

So, each remote project that is keyed in the resulting ResolvedIndices has at least one index that is mapped from the original index expression.

The thing I would like to verify is the assumption that we made in the slack thread, where we set the remote project's index in the OriginalIndices to be the original index expression and not the resolved index (e.g. ["logs*"] and not ["logs-1", "logs-2"].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing I would like to verify is the assumption that we made in the slack thread, where we set the remote project's index in the OriginalIndices to be the original index expression and not the resolved index

That's right. We want to associate with the user input index expressions and not the resolved values. You're on the right track.

import static org.hamcrest.Matchers.not;
import static org.mockito.Mockito.mock;

public class ResolvedIndicesTests extends ESTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ResolvedIndices.resolveWithIndexExpressions the method that ensures the right subset of indices are assigned to each linked project?

In other words, if I have say GET logs,foo,bar/_search and two linked projects p1 and p2, suppose these are the only indices that exist across all 3 projects:

_origin: logs
p1:foo
p2:bar

is ResolvedIndices.resolveWithIndexExpressions the method that ensures that each project alias is mapped to only the indices/aliases/datastreams found on each project?

If yes, I'd like to see that scenario tested here. I think you are only testing on one linked project and I'm having trouble following both what ResolvedIndices.resolveWithIndexExpressions does (the code is very dense) and what is being tested here. I think laying it out with code comments would help.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is ResolvedIndices.resolveWithIndexExpressions the method that ensures that each project alias is mapped to only the indices/aliases/datastreams found on each project?

kinda, from our discussion in the thread, it maps to the original index expression that resolves to the indices/aliases/datastreams found on each project.

"once we validated the flat expression we can use the rewritten index expression, so p1:logs* is fine no need to change it to p1:logs-1"

Copy link
Contributor

@pawankartik-elastic pawankartik-elastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with how the changes turned out. I'll approve it shortly.

Copy link
Contributor

@pawankartik-elastic pawankartik-elastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I don't see anything blocking my approval.

Copy link
Contributor

@quux00 quux00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - great work here Pat!

@prwhelan prwhelan merged commit b59dae9 into elastic:main Nov 26, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :Search Foundations/Search Catch all for Search Foundations serverless-linked Added by automation, don't add manually Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants